Code Review: Work Orders API — ID Filter & Show Action

Overview

Three files changed: added an id filter to the work orders query pipeline, fixed the show action to handle non-existent records, and added tests for both.

1. Status Codes: 401 vs 403

Finding: 401 is consistent with the codebase, but technically wrong per HTTP spec.

The chain works like this:

• Permissions.can/4 always returns {:error, :unauthorized} on policy failure
• FallbackController maps {:error, :unauthorized} → HTTP 401
• FallbackController maps {:error, :forbidden} → HTTP 403

Every API controller (RunController, JobController, ProjectController) follows this same pattern and returns 401 when an authenticated user tries to access a resource in a project they don't belong to. The only exception is CredentialController, which explicitly intercepts {:error, :unauthorized} and re-maps it to {:error, :forbidden} → 403.

Per RFC 7235, 401 means "not authenticated" and 403 means "authenticated but not authorized". The entire API layer has this backwards — but our test asserting 401 is consistent with the existing pattern. This is a pre-existing issue, not something introduced by this PR. If you want to fix it platform-wide, that's a separate effort.

2. Authorization Policies

Finding: Authorization is correct. The id filter cannot bypass access controls.

index (non-project-scoped): The base query work_orders_for(%User{}) inner-joins against the user's own projects via a subquery on project_users. The id filter adds a WHERE wo.id IN (...) on top of that already-scoped query. If someone passes a UUID for a work order in a project they don't belong to, the inner join eliminates it — they get an empty result, not the data.

index (project-scoped): Explicitly calls Permissions.can(:access_project, ...) before fetching any data.

show: Preloads workflow.project and calls Permissions.can(:access_project, ...). Non-existent IDs now correctly return 404 (the bug fix in this PR).

Role coverage: The :access_project policy (project_users.ex:52) grants access to any project member regardless of role (owner, admin, editor, viewer). Since work orders are read-only (:index and :show only in the router), this is appropriate — all members should be able to view execution status.

One gap worth noting: There is no test for the show action via the nested route (/api/projects/:project_id/work_orders/:id). The router defines both routes, but the show test only exercises the top-level route. This is low risk since both routes hit the same controller action.

3. Regression Risk Analysis

Risk: Very Low

No impact on:

• Other API endpoints (the filter function is only called from WorkOrdersController)
• The LiveView work order pages (they use different query paths)
• Background job processing (Oban workers don't use this query)
• The validate_datetime_params flow (untouched)

One minor code quality note: The id filter accepts arbitrary strings without UUID validation. Passing a non-UUID string like ?id=notauuid would hit Postgres with an invalid UUID cast and raise an Ecto error. The existing project_id and workflow_id filters have the same behavior, so this is consistent — but worth being aware of if you want to add input validation later.

Summary

• 401 is the established pattern — technically should be 403, but changing it here alone would be inconsistent. Flag for a future platform-wide fix if desired.
• Authorization is secure — the id filter cannot leak data across project boundaries.
• The show nil-guard is a genuine bug fix — the pre-existing code would crash on non-existent IDs.
• Regression risk is very low — all changes are additive or defensive.

Codecov Report

❌ Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.76%. Comparing base (b07826b) to head (5f8cd96).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4553      +/-   ##
==========================================
+ Coverage   89.64%   89.76%   +0.12%     
==========================================
  Files         444      444              
  Lines       21627    21660      +33     
==========================================
+ Hits        19387    19443      +56     
+ Misses       2240     2217      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Security Review

S0: Project-Scoped Data Access

• Status: PASS
• Findings:
  • New state filter for runs/work orders (lib/lightning/invocation/query.ex:146-157, 380-391) is applied on top of already-scoped base queries (work_orders_for/1, runs_for/1), which filter by the user's project memberships or the supplied project. No scoping bypass.
  • New work-order id filter (lib/lightning/invocation/query.ex:352-366) is applied on top of those same scoped base queries, so IDs belonging to projects the user cannot access are filtered out by the pre-existing where project_id / join … subquery(user.projects) clauses. Safe.
  • String.to_existing_atom in the state filter is gated by validate_run_state_param/validate_state_param (query.ex:164-181, 398-415), which whitelists against Run.states() / WorkOrder.states(). Both controller index actions call the validators before building the query (run_controller.ex:114,140, work_orders_controller.ex:121,147).
  • show action refactors in run_controller.ex:176-191, work_orders_controller.ex:182-198, project_controller.ex:95-109, and job_controller.ex:147-157 preserve the existing Permissions.can(:access_project, …) check after lookup. Cross-project access tests explicitly assert 401 (test/lightning_web/controllers/api/run_controller_test.exs:867-895, test/lightning_web/controllers/api/work_orders_controller_test.exs:690-708).
  • ai_assistant_controller.ex:101-131 refactor moves the project: :project_users preload into Jobs.get_job/2 but keeps the same Permissions.can(:workflows, :access_read, user, workflow.project) gate.
  • Note (non-blocking): show endpoints now distinguish 404 (nil record) from 401 (non-member). Since resource IDs are UUIDs, the existence-leak surface is negligible; callers' test expectations treat this as intentional.

S1: Authorization Policies

• Status: PASS
• Findings: No new mutating actions were introduced. Each modified controller action retains its pre-existing authorization call (Permissions.can(:access_project, …) or Permissions.can(:workflows, :access_read, …)). No new policy clauses needed.

S2: Audit Trail Coverage

• Status: N/A
• Findings: The PR adds only read-path filters, error-handling refinements, and documentation; no configuration resource is created, updated, or deleted. No audit module changes expected.

Summary

The PR is read-only (new state and id filters on list endpoints plus show-action error handling). All new filters compose on top of already project-scoped base queries, state validation rejects unknown atoms before String.to_existing_atom is called, and cross-project 401s are explicitly covered by new tests. No authorization or audit concerns.